-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate InstrumentationLibrary from metric.Descriptor #2197
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2197 +/- ##
=======================================
- Coverage 72.5% 72.5% -0.1%
=======================================
Files 168 168
Lines 11764 11872 +108
=======================================
+ Hits 8535 8609 +74
- Misses 2995 3029 +34
Partials 234 234
|
…y-go into jmacd/move_descriptor
@evantorrie please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through the PR, just wanted to share the feedback so far
…y-go into jmacd/move_descriptor
Co-authored-by: alrex <[email protected]>
LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with where it is, if you find you can remove the functions discussed above then 👍 to that too.
…o jmacd/move_descriptor
…rovider and InstrumentationLibraryReader directly, no need to get these
Please take a look at the sync.Map now used as the MeterProvider in |
Co-authored-by: Anthony Mirabella <[email protected]>
…y-go into jmacd/move_descriptor
This is ready to merge, I think. Thanks all for your patience. |
@MadVikingGod I reverted two commits that changed the controller to use a sync.Map behind its MeterProvider implementation. This can be addressed in a future PR. I've left a TODO about it, the spec will require this sort of improvement. I propose any further changes to this PR be left as TODOs, but I think it's ready to merge as-is. |
I have talked with @jmacd offline about this, and agree that the sync.Map code is better off as an improvement in a later PR. Count this as reapproving this from me. |
Thank you, approvers & maintainers. This was a tough one, I appreciate your patience. 🚀 |
Please excuse a relatively large PR. I've made efforts to keep it easy-to-review as described below.
The problem being addressed is that the
metric.Descriptor
contains the instrumentation library name and version, which force consumers of the metric data to re-group metrics by instrumentation library.One goal is to move the Descriptor type into the new
metric/sdkapi
sub-package, however that move will only create a larger PR so this PR leaves it in place and a future, simple PR will apply the move. We had to refactor this because theInstrumentOption
andMeterOption
types do not belong insdkapi
. Thus,NewDescriptor
needed to lose those options.The
metric.NewDescriptor()
function was problematic because it took bothInstrumentOption
andMeterOption
, and our goal is to disentangle these. A few valid non-test uses ofNewDescriptor
are modified to manage theInstrumentOption
values themselves, i.e., compute an InstrumentConfig and extract the two options (description and unit) manually. All the test-only uses ofmetric.NewDescriptor()
are replaced bymetrictest.NewDescriptor()
with the old behavior (i.e., accepting varargs options for description and unit). This minimizes the size of this PR.The metric Controller now maintains one Accumulator per instrumentation library, as opposed to one Accumulator per SDK. This changes the export path, giving it a two-level structure where the exporter sees one instrumentation library at a time, then can visit each export record.
The name
CheckpointSet
became problematic and was never a good name. "Checkpoint" is the write-oriented behavior that is carried out by the metric Processor; "Reader" is the read-oriented behavior carried out by the exporter. The existingCheckpointSet
has been renamedReader
. A new type namedInstrumentationLibraryReader
is introduced to read one library at a time, which gives a two-levelForEach()
style of iteration.The Controller now takes a factory for building processors instead of a single processor, thus many constructors change from
controller.New(processor.New(...), ...)
tocontroller.New(processor.NewFactory(...), ...)
.The real, functional changes in this PR are:
metric/registry
a new*Provider
type maintains a unique-map ofMeter
implementations, separate from the unique-map of instrument/kind that it already kept. The new*Provider
wraps around an implementation ofMeterProvider
that is not required to maintain name-to-Meter uniqueness, whereas before this was encapsulated in a single unique map.internal/metric/global
a new*Provider
type defers MeterProvider initialization explicitly, whereas before the uniqueness was implicitly managed by the registrysdk/metric/controller/basic
, now using the registry Provider, which exposes aList()
method to access the registered instrumentation libraries, to export two-level information through the newInstrumentationLibraryReader
. there is a slight change of lock semantics; whereas before the Controller used its one Accumulator's state as a lock to determine when refresh was needed, it now uses the same lock used to protect against multiple Start()/Stop() calls; no significant change of behavior hereexporters/otlp/otlpmetric/...
the grouping by instrumentation library is removed; calls tosource()
andsink()
are carried out once per instrumentation library.A bit of test cleanup has been applied. There were a number of test-only implementations of the former
CheckpointSet
interface that were consolidated into helpers insdk/metric/processor/processortest
.Part of #2090.
Fixes #2086.
Part of #2119.
File-by-file review notes (non-test files): #2197 (comment)